feat(k8s): Make RunContainer detect/report more error conditions like image pull errors #331

cognifloyd · 2022-04-30T00:52:35Z

This is an alternate implementation of #279 using the SharedInformer infra to watch events introduced in #519.
Closes #279

Overview

The Docker Runtime surfaces image pull errors naturally because of the blocking synchronous requests to pull an image or start a container.

The Kubernetes Runtime has to watch for events that show any errors with pulling images.

`RunContainer` changes in k8s runtime

Wait until one of these conditions before returning:

build canceled
container is running
got an image pull error

`WaitContainer` changes in k8s runtime

Wait until one of these conditions before returning:

build canceled (new)
container is terminated (was already doing this)

`containerTracker` additions

I added signal channels for Running and ImagePulled. I also added a channel for receiving image pull errors in RunContainer.

`podTracker` changes

I need pod Name and Namespace in more places, so I copy those into the podTracker now.

I was surprised that the SharedInformerFactory I used to create the Informer/Lister for watching the build Pod did NOT work for watching events. Apparently, events don't get labeled, so I had to add a separate eventInformerFactory that uses a FieldSelector instead of a LabelSelector when making the list/watch API calls.

I did not rename informerFactory to podInformerFactory because it can easily be used for other k8s resources in the future (if needed), just not for Events.

I also added the Event event handler funcs (wow that was a mouth full - events about Events).

podTracker.inspectContainerEvent() (called by the Event event handlers) is responsible for

sending the image pull events to RunContainer
closing the ImagePulled signal channel when an event indicates that the pull was successful

I also adjusted podTracker.inspectContainerStatuses() so that it can also:

surface image pull errors (in case the error is visible there, but not through the Events API - which is unlikely, but there doesn't seem to be any kind of guarantees about event availability, so this covers our bases)
close the Running signal channel when the container status shows that it is running.

TODO:

merge enhance(kubernetes): Add podTracker and containerTracker to use k8s API more like a k8s controller #302
rebase
write tests

codecov · 2022-05-03T20:12:33Z

Codecov Report

Merging #331 (65cbe59) into master (a9cbe48) will increase coverage by 1.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #331      +/-   ##
==========================================
+ Coverage   81.18%   82.20%   +1.01%     
==========================================
  Files          68       68              
  Lines        5273     5472     +199     
==========================================
+ Hits         4281     4498     +217     
+ Misses        848      831      -17     
+ Partials      144      143       -1

Impacted Files	Coverage Δ
runtime/kubernetes/build.go	`88.60% <100.00%> (+1.55%)`	⬆️
runtime/kubernetes/container.go	`91.64% <100.00%> (+9.61%)`	⬆️
runtime/kubernetes/pod_tracker.go	`97.37% <100.00%> (+1.51%)`	⬆️

We have nothing to do when that happens.

we have to use different list options, so they can't share the same factory.

…ect image

This needs access to the PodTracker's EvenLister, so we create the function when initializing the containerTracker.

I've documented a bunch of other event reasons to facilitate expanded event handling in the future. Also, I keep running across these messages, so this help me remember what they are and where they come from.

Now that RunContainer listens for k8s-state changes, the tests need to actually simulate the state change like the WaitContainer tests do.

…age)

I'll re-add it later when needed.

kneal · 2022-05-05T16:06:48Z

runtime/kubernetes/container.go

+	// known kubelet event reasons are listed here:
+	// https://github.com/kubernetes/kubernetes/blob/v1.23.6/pkg/kubelet/events/event.go
+
+	// kubelet image event reasons.


Would it make sense to just use their constants or wrap them at least so we know when they change?

They have not packaged the code so that it can be used externally.
https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/events/event.go

If I do go get github.com/kubernetes/kubernetes I get an error module declares its path as: k8s.io/kubernetes
If I do go get k8s.io/kubernetes I get an error that k8s.io/[email protected] requires k8s.io/[email protected]: reading k8s.io/api/go.mod at revision v0.0.0: unknown revision v0.0.0

They seem to carefully curate what can be imported by external projects, so we can't import this.

Cool, just checking. I just noticed when I went to the code they were public on the package. So, thought they might be importable.

Thought I'd provide a bit of clarity on this subject...

The tl;dr is you can vendor from k8s.io/kubernetes but its not pretty or straightforward 😅

Here's the reason why go get k8s.io/kubernetes fails:

kubernetes/kubernetes#79384 (comment)

And to actually vendor from k8s.io/kubernetes, you have to add a replace directive for all nested packages:

kubernetes/kubernetes#79384 (comment)

If we want, there are people who've scripted the approach to this so updating the version is easier:

kubernetes/kubernetes#79384 (comment)

To see a real world example of what this looks like in the go.mod file:

https://github.com/kubernetes-sigs/cloud-provider-huaweicloud/blob/9589bc854c29e399476479f9c5aa9599b2064da5/go.mod#L18-L40

Eww... All that work just to reuse some constants... Not worth it imo.

Agreed 😄

runtime/kubernetes/pod_tracker.go

kneal

LGTM 🐬

jbrockopp

LGTM

cognifloyd · 2022-05-07T17:42:32Z

I'm running into some issues with this, so I'm moving it back to draft for now.

cognifloyd added the bug Indicates a bug label Apr 30, 2022

cognifloyd self-assigned this Apr 30, 2022

cognifloyd added feature Indicates a new feature and removed bug Indicates a bug labels Apr 30, 2022

cognifloyd changed the title ~~K8s events~~ feat(k8s): Make RunContainer detect/report more error conditions like image pull errors. Apr 30, 2022

cognifloyd changed the title ~~feat(k8s): Make RunContainer detect/report more error conditions like image pull errors.~~ feat(k8s): Make RunContainer detect/report more error conditions like image pull errors Apr 30, 2022

cognifloyd mentioned this pull request May 2, 2022

feat(kubernetes): Surface Image Pull Errors in Kubernetes Runtime #279

Closed

copart-jafloyd force-pushed the k8s-events branch 2 times, most recently from 2d5034b to f33af7f Compare May 3, 2022 20:08

copart-jafloyd force-pushed the k8s-events branch 2 times, most recently from abdb440 to bc846e0 Compare May 4, 2022 18:31

cognifloyd added 18 commits May 4, 2022 13:32

feat(k8s): add eventInformer to podTracker

2929949

feat(k8s): ignore event deletion

be144f4

We have nothing to do when that happens.

feat(k8s): begin handling event stream

7602600

refactor: create eventInformer from eventInformerFactory

457a664

we have to use different list options, so they can't share the same factory.

refactor: rename selector=>labelSelector

7d674e8

enhance: register eventInformerFactory on podTracker

71c9b34

enhance: add podTracker.inspectContainerEvent

c3e4b50

enhance: add signal for running container

3e89df6

enhance: only mark containers as running/terminated if it is the corr…

8ba7ad5

…ect image

enhance(k8s): exit WaitContainer if build is canceled

038bf97

enhance(k8s): add containerTracker.Events() function

b5ab8eb

This needs access to the PodTracker's EvenLister, so we create the function when initializing the containerTracker.

tests: fix inspectContainerStatuses test

57011c1

bugfix(k8s): Make sure image pull errors are detected

639b219

refactor(k8s): use channels to signal imagePull success/errors

092840b

fix: comment typos

e3d6d93

enhance: capture ImagePull errors from ContainerStatuses as well

aa1078e

enhance(k8s): handle more image pull event types

9c64abd

I've documented a bunch of other event reasons to facilitate expanded event handling in the future. Also, I keep running across these messages, so this help me remember what they are and where they come from.

tests(k8s): fix tests for RunContainer

9686c23

Now that RunContainer listens for k8s-state changes, the tests need to actually simulate the state change like the WaitContainer tests do.

cognifloyd added 13 commits May 4, 2022 13:32

tests(k8s): test RunContainer and WaitContainer with canceled build

dd189e0

tests(k8s): test AssembleBuild with canceled build

ca14e32

tests(k8s): test RunContainer with PodTracker failure (increase cover…

2a6d109

…age)

tests(k8s): test inspectContainerStatuses with Running or ImagePullError

08c219c

chore: prune some comments

40da3f8

tests: fix inspectContainerStatuses test with an errgroup

4726072

tests: test RunContainer with an ImagePullError

0d2f315

tests: test getTrackedPodEvent

ccf9fb2

tests: test podTracker.HandleEventAdd, podTracker.HandleEventUpdate

f76339d

refactor: drop unused Events function

e6876d1

I'll re-add it later when needed.

tests: test inspectContainerEvent image pull events

ecceeac

tests: test inspectContainerEvent edge cases

b945f33

tests: test more pull policies in SetupContainer

9ec81fa

copart-jafloyd force-pushed the k8s-events branch from bc846e0 to 9ec81fa Compare May 4, 2022 18:44

cognifloyd marked this pull request as ready for review May 4, 2022 18:56

cognifloyd requested a review from a team as a code owner May 4, 2022 18:56

cognifloyd requested review from jbrockopp and kneal May 4, 2022 18:56

kneal requested changes May 5, 2022

View reviewed changes

chore: delete dead code.

65cbe59

kneal approved these changes May 5, 2022

View reviewed changes

jbrockopp approved these changes May 5, 2022

View reviewed changes

cognifloyd marked this pull request as draft May 7, 2022 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(k8s): Make RunContainer detect/report more error conditions like image pull errors #331

feat(k8s): Make RunContainer detect/report more error conditions like image pull errors #331

cognifloyd commented Apr 30, 2022 •

edited

Loading

codecov bot commented May 3, 2022 •

edited

Loading

kneal May 5, 2022

cognifloyd May 5, 2022

kneal May 5, 2022

jbrockopp May 5, 2022 •

edited

Loading

cognifloyd May 5, 2022

jbrockopp May 6, 2022

kneal left a comment

jbrockopp left a comment

cognifloyd commented May 7, 2022

feat(k8s): Make RunContainer detect/report more error conditions like image pull errors #331

Are you sure you want to change the base?

feat(k8s): Make RunContainer detect/report more error conditions like image pull errors #331

Conversation

cognifloyd commented Apr 30, 2022 • edited Loading

Overview

RunContainer changes in k8s runtime

WaitContainer changes in k8s runtime

containerTracker additions

podTracker changes

codecov bot commented May 3, 2022 • edited Loading

Codecov Report

kneal May 5, 2022

Choose a reason for hiding this comment

cognifloyd May 5, 2022

Choose a reason for hiding this comment

kneal May 5, 2022

Choose a reason for hiding this comment

jbrockopp May 5, 2022 • edited Loading

Choose a reason for hiding this comment

cognifloyd May 5, 2022

Choose a reason for hiding this comment

jbrockopp May 6, 2022

Choose a reason for hiding this comment

kneal left a comment

Choose a reason for hiding this comment

jbrockopp left a comment

Choose a reason for hiding this comment

cognifloyd commented May 7, 2022

cognifloyd commented Apr 30, 2022 •

edited

Loading

`RunContainer` changes in k8s runtime

`WaitContainer` changes in k8s runtime

`containerTracker` additions

`podTracker` changes

codecov bot commented May 3, 2022 •

edited

Loading

jbrockopp May 5, 2022 •

edited

Loading